feat: Screen Config Postgres Client APIs and Admin Logic - #3231
feat: Screen Config Postgres Client APIs and Admin Logic#3231robbie-sundstrom wants to merge 10 commits into
Conversation
04ba74c to
6607c7f
Compare
| end) | ||
| end | ||
|
|
||
| @doc """ |
There was a problem hiding this comment.
For my own understanding, we have a mix of @doc comments and blocks of comments (e.g.). How do we determine what we want to doc with @doc vs not?
There was a problem hiding this comment.
I do need to be consistent with this 😅 I kind of like the @doc comments because of how they integrate with tools like ElixirLS in VSCode. But I do generally think the @doc comments are moreso for API consumers, which is why we generally don't have them throughout our codebase. I'll move towards consistency here, which I'd like to be @doc comments for this new file at least
There was a problem hiding this comment.
The Elixir convention as far as I've seen is @doc for public functions and # blocks for private functions, since private functions are treated as "hidden" anyway and there's no way to access docs attached to them beyond looking at the code. Technically the main purpose of @doc is in generating ExDocs which are made available on Hex for published libraries; in an application codebase, it's mostly useful for editor LS integration. I usually include them if there's something important for callers to know that isn't covered by the function name, argument names, and typespec, which is all also readily accessible via LSP. (This is not just "for API consumers" in the sense of "consumers of some API external to the whole application", but if you take an expansive view of an "API" as "the set of public functions exported by a module", then that is accurate!)
26fc1a9 to
bc959bf
Compare
bc959bf to
cafbc44
Compare
06876e5 to
f12a489
Compare
rwaskiewicz
left a comment
There was a problem hiding this comment.
LGTM! Thanks for making those changes. Two non-blocking comments
| onConfirm: (config: any) => Promise<{ success: boolean; error?: string }>; | ||
| configRef: RefObject<HTMLTextAreaElement | null>; | ||
| onCancel: () => void; | ||
| onError: (string) => void; |
There was a problem hiding this comment.
Non-blocking nit - it looks like the parameter name (or type, depending how you look at it) here is omitted:
| onError: (string) => void; | |
| onError: (errorMsg: string) => void; |
But looking at it's usage here, maybe its type is more than that of type string?
screens/assets/src/components/admin/admin_form.tsx
Lines 77 to 81 in f12a489
where in the else clause, result.error can be undefined, and if the catch clause, error is of type unknown. Keeping the API boundary between components the same, one potential type-safe solution could be:
if (result.success === true) {
onSuccess();
} else if (result.error) {
onError(result.error);
} else {
onError(`An unknown exception occurred updating the config`);
}
} catch (error: unknown) {
if (error instanceof Error) {
onError(error.message);
} else if (error && typeof error === 'object' && 'toString' in error) {
onError(error.toString())
} else {
onError(`An unknown exception occurred....we would need to make this distinct from the other exception where we have no idea what happened`);
}
}There was a problem hiding this comment.
you're right! Parsing these unknown/undefined errors into strings seems like a good solution to me
Asana task: Define Screen Config shared APIs
indexScreens Admin endpoint now calls a different function that handles the feature flag logicscreen_configsendpoint that handles modifying/deleting screen configs in JSON or Postgres based on the feature flagFrontend Changes
commitScreenConfigChanges.editor.tsxcalls this directlyinspector.tsxnow calls this within theAdminFormcomponent.